Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make ScalaNativeModule use Scala 2.13 toolchain when available #1693

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Make ScalaNativeModule use Scala 2.13 toolchain when available #1693

merged 3 commits into from
Jan 25, 2022

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Jan 22, 2022 •
edited
Loading

Motivation

  • Consistency with ScalaJSModule
  • Not resolving Scala 2.12 if not needed (imagining a Scala 2.13 only project) could make the build faster
  • Not loading a new classloader from scratch and populating it with a brand new scala-library for Scala 2.12 could be faster (I have no data to prove this theory)

@lolgab lolgab mentioned this pull request Jan 22, 2022
Merged
@lolgab
Make ScalaNativeModule use Scala 2.13 toolchain
1eabfc3 
- Drop Support for Scala Native 0.4.0 and 0.4.1
@lolgab
Do not use null parent classloader
12d1585 
Now that Scala Native is using Scala 2.13 we can avoid using the null
parent classloader since it can share the same scala-library with Mill
@lolgab
Copy link
Member Author

lolgab commented Jan 23, 2022 •
edited
Loading

Tested with Scala Native 0.4.1 and it fails with:

1 targets failed
module.bridgeFullClassPath 
Resolution failed for 2 modules:
--------------------------------------------
  org.scala-native:test-runner_2.13:0.4.1 
        not found: /Users/lorenzo/.ivy2/local/org.scala-native/test-runner_2.13/0.4.1/ivys/ivy.xml
        not found: https://repo1.maven.org/maven2/org/scala-native/test-runner_2.13/0.4.1/test-runner_2.13-0.4.1.pom
  org.scala-native:tools_2.13:0.4.1 
        not found: /Users/lorenzo/.ivy2/local/org.scala-native/tools_2.13/0.4.1/ivys/ivy.xml
        not found: https://repo1.maven.org/maven2/org/scala-native/tools_2.13/0.4.1/tools_2.13-0.4.1.pom

Given the very low usage of Scala Native + Mill I'd say this error is good enough, because you just need to follow the maven link to see that the first tools_2.13 and test-runner_2.13 artifacts are of version 0.4.2:
https://repo1.maven.org/maven2/org/scala-native/test-runner_2.13/

Also Scala Native 0.4.3 adds Scala 3 support so it is likely everyone will migrate to it very very soon.

Edit

The PR was edited to not drop the support for Scala Native 0.4.0 and 0.4.1 by crosscompiling the worker to both 2.12 and 2.13 versions.

@lolgab lolgab marked this pull request as ready for review January 23, 2022 13:45
@lolgab lolgab requested a review from lefou January 23, 2022 15:57
lefou
lefou reviewed Jan 25, 2022
View reviewed changes
Copy link
Member

@lefou lefou left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'm not using Scala Native myself, I have no deep understanding how people are using it and how easy it is to upgrade. Are there any real issues for those still using Mill + SN 0.4.0 or 0.4.1 to just use 0.4.2 instead? Can they just bump and are good to go, or are there potential incompatibilities and required changes? Are they forced to also have all dependencies released for at least 0.4.2?

If there are no real hurdles for SN users to upgrade, than your PR is reasonable. Otherwise, we should consider introducing just a new second worker which is essentially what you provided here, and keep the old one for those forced to stay a bit longer with SN 0.4.{0,1}. I think the arguments to avoid loading another Scala version with comes at a time and memory costs sounds nice, but should not drive the decision, if there is also potential we loose some users.

And finally a note to myself: We should invite users to add theirself to some users list, so we have a better understanding how many users are using which features of mill. Unfortunately, as I can't access the Sonatype staging group, I can't see the download statistics for those artifacts.

@lolgab
Copy link
Member Author

lolgab commented Jan 25, 2022 •
edited
Loading

After reading your comment I added a commit that crosscompiles the worker and uses the Scala 2.12 one only with Scala Native 0.4.0 and 0.4.1. This way the change is non breaking anymore. If you like the changes it is ready to merge from my side.

@lolgab lolgab changed the title Make ScalaNativeModule use Scala 2.13 toolchain Make ScalaNativeModule use Scala 2.13 toolchain when available Jan 25, 2022
@lolgab lolgab requested a review from lefou January 25, 2022 15:39
lefou
lefou approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I appreciate the efforts you made to keep compatibility with SN 0.4.0.

@lefou lefou merged commit 6e9327a into main Jan 25, 2022
@lefou lefou added this to the after 0.10.0 milestone Jan 25, 2022
@lefou
Copy link
Member

lefou commented Jan 25, 2022

I merged via merge commit, to preserve the result of having only one Scala 2.13 based worker.

@lolgab lolgab deleted the scala-native-scala-213 branch January 25, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lefou lefou lefou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.1
Development

Successfully merging this pull request may close these issues.
2 participants
@lolgab @lefou